Skip to content

Add lozalization to more-info-vacuum#4793

Merged
bramkragten merged 8 commits into
home-assistant:devfrom
edenhaus:more-info-vacuum
Feb 11, 2020
Merged

Add lozalization to more-info-vacuum#4793
bramkragten merged 8 commits into
home-assistant:devfrom
edenhaus:more-info-vacuum

Conversation

@edenhaus
Copy link
Copy Markdown
Member

@edenhaus edenhaus commented Feb 6, 2020

Proposed change

  • Converted more-info-vacuum to Lit
  • Introduced VacuumCommand, so we can use a stream to create the command icons
  • Add localization to more-info-vacuum

Not translated

I have not translated stateObj.attributes.status, because this attribute can be any value.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

vacuum:
  - platform: demo

Additional information

I'm not sure if introducing the new class VacuumCommand was correct, but in my opinion it makes the code more clear and understandable.
I have separated each proposed into a separated commit, so it should be easier to see the different changes.
As this is my first pullrequest in this project, feel free to changes or to give me feedback :)

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @edenhaus,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@edenhaus edenhaus changed the title More info vacuum Add lozalization to more-info-vacuum Feb 6, 2020
Comment thread src/dialogs/more-info/controls/more-info-vacuum.ts Outdated
Comment on lines +116 to +121
const supportsFanSpeed = supportsFeature(
stateObj,
VACUUM_SUPPORT_FAN_SPEED
);
const supportsBattery = supportsFeature(stateObj, VACUUM_SUPPORT_BATTERY);
const supportsStatus = supportsFeature(stateObj, VACUUM_SUPPORT_STATUS);
Copy link
Copy Markdown
Member

@bramkragten bramkragten Feb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we use these only once, it is nicer to inline the check instead of creating a const.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to inline, except for supportsCommandBar as in my opinion it is clearer if it is a variable and not checked inline.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I only meant the ones that were used once, the ones that are used twice (in supportsCommandBar) should be a const.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today I had another idea, I can use VACUUM_COMMANDS.some(...) so we don't have any duplicate code and the maintenance of the code is easier.

Copy link
Copy Markdown
Member

@bramkragten bramkragten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks. Just a few small remarks.

- Add different translation for start_pause and pause as they are not the same
@edenhaus
Copy link
Copy Markdown
Member Author

edenhaus commented Feb 8, 2020

The button pause and play_pause had the same tooltip "Pause" (before this PR). I changed the tooltip of play_pause to "Start/Pause"

Comment thread src/dialogs/more-info/controls/more-info-vacuum.ts Outdated
Comment thread src/dialogs/more-info/controls/more-info-vacuum.ts Outdated
Comment thread src/dialogs/more-info/controls/more-info-vacuum.ts Outdated
Comment thread src/dialogs/more-info/controls/more-info-vacuum.ts Outdated
Comment thread src/dialogs/more-info/controls/more-info-vacuum.ts Outdated
Comment thread src/dialogs/more-info/controls/more-info-vacuum.ts Outdated
@edenhaus
Copy link
Copy Markdown
Member Author

Thanks for your suggestions :)

@bramkragten bramkragten merged commit 9fce600 into home-assistant:dev Feb 11, 2020
@bramkragten bramkragten mentioned this pull request Feb 12, 2020
@lock lock Bot locked and limited conversation to collaborators Feb 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants